Add support for OIDC session management error status #613
Conversation
divid3byzero
changed the title
|
|
|
You are right, sorry, I didn't commit everything. I messed up my local development and forgot a few things in the commit after fixing it. I've committed the missing code. Actually there are use cases when clients want to react to the error status. And as that is something different than the changed status I (and everyone else that wants to react differently to the two statuses) can't use |
src/CheckSessionIFrame.ts
Outdated
| @@ -52,9 +54,13 @@ export class CheckSessionIFrame { | |||
| ) { | |||
| if (e.data === "error") { | |||
| this._logger.error("error message from check session op iframe"); | |||
| if (this._stopOnError) { | |||
| if (this._stopOnError && !this.propagateUserSessionError) { | |||
There was a problem hiding this comment.
if _stopOnError is true i really would expect that this.stop() should be called. You need to rearrange your code changes
divid3byzero
force-pushed
the
Issue610_error_status
branch
from
b609b64
to
40fb449
Compare
| // catch to suppress errors since we're in non-promise callback | ||
| logger.error("Error from getCheckSessionIframe:", err instanceof Error ? err.message : err); | ||
| } | ||
| }; | ||
|
|
||
| private doUserSessionErrorPropagation(): boolean { |
There was a problem hiding this comment.
doUserSessionErrorPropagation is only called once -> not really needed, instead add there e.g. a variable
There was a problem hiding this comment.
This is a matter of preference, but yes, I will introduce a variable.
| @@ -55,6 +57,10 @@ export class CheckSessionIFrame { | |||
| if (this._stopOnError) { | |||
| this.stop(); | |||
| } | |||
|
|
|||
| if (this.propagateUserSessionError) { | |||
There was a problem hiding this comment.
I guess we could combine propagateUserSessionError and userSessionErrorCallback: Make userSessionErrorCallback optional, if defined call it if not not. Then propagateUserSessionError is not needed inside this class...
There was a problem hiding this comment.
Actually the way I implemented it is the same pattern as the _callback is called after the changed status is triggered. By using the propagateUserSessionError flag it is made optional. If you'd rather like that the existence of the userSessionErrorCallback is checked to trigger that callback I would have to allow for a function to be defined in the settings object which, of course, would be possible but would break with the way that a client can attach to e.g. the changed status.
Codecov Report
@@ Coverage Diff @@
## main #613 +/- ##
==========================================
- Coverage 76.91% 76.71% -0.21%
==========================================
Files 44 44
Lines 1590 1602 +12
Branches 299 301 +2
==========================================
+ Hits 1223 1229 +6
- Misses 336 342 +6
Partials 31 31
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Nice work! |
Closes/fixes #611
Checklist